-
-
Notifications
You must be signed in to change notification settings - Fork 550
Bring back conversion of process CPU time on macOS (#1638) #1691
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
2e58de2 to
fb4394c
Compare
fb4394c to
4d352ea
Compare
4d352ea to
fa06db1
Compare
fa06db1 to
884bff9
Compare
|
I don't have any more comment with this code now. But perhaps is good to explain what's the difference between this PR and #1643. Does this supersede #1643? Cc @aestriplex |
884bff9 to
7e979ab
Compare
|
Is this fix expected to be merged for next release? I just had to copy old 3.3.0 htop from another mac to mine in order to make it show the right CPU usage. Something like this that makes the software fail to a point it's not showing the right data should be fixed much quicker than several months, as meanwhile it makes it unreliable and unusable for a quite common use-case, which might make users look for alternatives instead, as having the code done right is useless if the product does not work as intended. |
7e979ab to
63a0097
Compare
|
Rebased on main (comment added to prevent GitHub from merging multiple force pushes) |
63a0097 to
d086bec
Compare
|
@Korrd Please keep in mind I'm just a volunteer. While I can help evaluting the quality of the patches, I don't have all day to do it and I have no guarantee on which patch would be reviewed sooner or later. (And I'm not the maintainer of this project - this is just my personal position and not the maintainers'.) |
@Explorer09 yes, this PR supersede #1643. If it's one with anyone, I can close it |
darwin/PlatformHelpers.h
Outdated
| // lowerBound <= currentVersion < upperBound | ||
| bool Platform_KernelVersionIsBetween(KernelVersion lowerBound, KernelVersion upperBound); | ||
|
|
||
| void Platform_calculateNanosecondsPerMachTick(uint64_t* numer, uint64_t* denom); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you are only calling this Platform_calculateNanosecondsPerMachTick() from Platform_init(), I wonder if it's possible to make the former function static scope and not expose this as a public interface in PlatformHelpers.h?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case I would have to move it to Platform.c as it's in another compilation unit otherwise. That file is already rather long. Should I?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no problem with that. Especially as this allows to reduce visibility/exposed interface for a function that is unlikely to get re-used. And regarding large files: There are far more complex ones. And just for comparison: The platform implementation (main part) for Linux is twice the size … What matters more is keeping code that belongs together and is used as an unit near each other; thus with this just being called on platform initialization and all other code related to timebase conversion in Platform.c it's more logical to also have this function over there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, pushed change.
BenBE
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two minor. Rest seems fine.
|
I slightly reordered the logic of the static void Platform_calculateNanosecondsPerMachTick(uint64_t* numer, uint64_t* denom) {
// Check if we can determine the timebase used on this system.
// If the API is unavailable assume we get our timebase in nanoseconds.
*numer = 1;
*denom = 1;
#ifdef __x86_64__
/* WORKAROUND for `mach_timebase_info` giving incorrect values on M1 under Rosetta 2.
* rdar://FB9546856 http://www.openradar.appspot.com/FB9546856
*
* We don't know exactly what feature/attribute of the M1 chip causes this mistake under Rosetta 2.
* Until we have more Apple ARM chips to compare against, the best we can do is special-case
* the "Apple M1" chip specifically when running under Rosetta 2.
*
* Rosetta 2 only supports x86-64, so skip this workaround when building for other architectures.
*/
bool isRunningUnderRosetta2 = Platform_isRunningTranslated();
// Kernel version 20.0.0 is macOS 11.0 (Big Sur)
bool isBuggedVersion = 0 <= Platform_CompareKernelVersion((KernelVersion) {20, 0, 0});
if (isRunningUnderRosetta2 && isBuggedVersion) {
// In this case `mach_timebase_info` provides the wrong value, so we hard-code the correct factor,
// as determined from `mach_timebase_info` as if the process was running natively.
*numer = 125;
*denom = 3;
return;
}
#endif
#ifdef HAVE_MACH_TIMEBASE_INFO
mach_timebase_info_data_t info;
if (mach_timebase_info(&info) == KERN_SUCCESS) {
*numer = info.numer;
*denom = info.denom;
return;
}
#endif
}I feel the code path like this would be easier to follow. LGTM for other parts of the code. |
Minor caveat, as with the current way the static void Platform_calculateNanosecondsPerMachTick(uint64_t* numer, uint64_t* denom) {
// Check if we can determine the timebase used on this system.
#ifdef __x86_64__
/* WORKAROUND for `mach_timebase_info` giving incorrect values on M1 under Rosetta 2.
* rdar://FB9546856 http://www.openradar.appspot.com/FB9546856
*
* We don't know exactly what feature/attribute of the M1 chip causes this mistake under Rosetta 2.
* Until we have more Apple ARM chips to compare against, the best we can do is special-case
* the "Apple M1" chip specifically when running under Rosetta 2.
*
* Rosetta 2 only supports x86-64, so skip this workaround when building for other architectures.
*/
bool isRunningUnderRosetta2 = Platform_isRunningTranslated();
// Kernel version 20.0.0 is macOS 11.0 (Big Sur)
bool isBuggedVersion = 0 <= Platform_CompareKernelVersion((KernelVersion) {20, 0, 0});
if (isRunningUnderRosetta2 && isBuggedVersion) {
// In this case `mach_timebase_info` provides the wrong value, so we hard-code the correct factor,
// as determined from `mach_timebase_info` as if the process was running natively.
*numer = 125;
*denom = 3;
return;
}
#endif
#ifdef HAVE_MACH_TIMEBASE_INFO
mach_timebase_info_data_t info = { 0 };
if (mach_timebase_info(&info) == KERN_SUCCESS) {
*numer = info.numer;
*denom = info.denom;
return;
}
#endif
// No info on actual timebase found; assume timebase in nanoseconds.
*numer = 1;
*denom = 1;
}That way, the fallback is clearly visible. This also highlights, that both |
d086bec to
a01af46
Compare
I applied the suggested fixes |
a01af46 to
e1471e9
Compare
e1471e9 to
cd027be
Compare
|
Thanks a lot guys! <3 My love to you all! |
Fixes #1638